feat(executor): caller-owned deadline, lockers, and backend PID for concurrent builds - #76
Conversation
…oncurrent builds An orchestrator holding a concurrent index build under a renewable lease cannot express its bound as a fixed statement_timeout. Let ConcurrentBudget opt into caller-owned mode, where the cancellable context is the statement's only bound (a non-cancellable context is refused), expose the build backend PID through the progress tracker so the build can be cancelled from a second connection, and add lockers to the progress snapshot so a stalled build shows what it waits on.
382caea to
5209138
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head Verdict: caller-owned mode is bounded by construction and the new refusals hold — but the mode's own primary exit path is the one thing not covered: a build cancelled by its caller can report
Findings1. In caller-owned mode every SQLSTATE 57014 becomes 2. 3. (test) Deleting the 4. (nit) The PR body says Action items
Verified (tried to break these, couldn't)Mutation results: deleting the This review was generated by Claude Code (claude-opus-5). |
|
🤖 Two-lens product review (adoption + integration), same head Lens 1 — OSS adoption easeThe best thing in this PR is a refusal message.
The Lens 2 — SchemaBot integrationThe seam is right to design now, because there is no consumer yet. SchemaBot's The version bump is correctly scoped and does not gate anything downstream. SchemaBot's strict-version gate is What integration will actually need next is a reason it can classify, not just an error it can wrap. Caller-owned mode exists for an orchestrator that renews a lease, and that orchestrator's decision tree is: my lease lapsed (retry later, same plan), an operator stopped it (do not retry, tell the human), the server killed it (retry with a different bound). Today the first two collapse into This review was generated by Claude Code (claude-opus-5). |
aparajon
left a comment
There was a problem hiding this comment.
🤖 Approving on Armand's behalf after the adversarial correctness review above. The findings there are yours to pick up as follow-ups — flagging them, not gating on them.
This stamp was left by Claude Code (claude-opus-5).
|
🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/76, follow-up commit All five correctness findings and both two-lens prose findings are fixed in the follow-up commit; the progress/status CLI surface is noted as future work, not this PR.
Verified correct (refusal ordering before session acquisition, Source: #76, review comments 5536112496 and 5536112877 |
…on the tracker (#78) Follow-up to #76: the caller's own cancellation of a concurrent index build is reported as `cancelled-by-caller` in both modes, and the operator's stop path becomes `Tracker.CancelBuild` instead of a handed-out backend PID. ## Why #76 merged with its caller-owned mode but without the fixes from its adversarial review. Two of those are correctness gaps an orchestrator would hit immediately. In caller-owned mode every SQLSTATE 57014 was read as `ErrCancelledExternally`, including the caller's own cancellation whenever the server's ErrorResponse arrived before pgx's local context error — so a lease that lapsed looked like an operator's intervention, and the orchestrator's retry decision was wrong. And `Tracker.BuildPID()` handed out a PID with no validity window: a caller holding a stale one could `pg_cancel_backend` whatever the pool next ran on that backend. ## What - `asConcurrentBudgetError` takes the caller's context. A server cancellation at or past the overall budget is a `*BudgetError` whatever the caller's context did meanwhile — an orchestrator's deadline commonly sits just outside the budget it configured, and the escalation signal must survive that coincidence. Below the budget, an ended caller context is `ErrCancelledByCaller` (`cancelled-by-caller`) in either mode, whichever side of the race wins; a 57014 under a live context stays `ErrCancelledExternally` ("from outside the executor"). `corroborateValidateCancel` gets the same three-way typing, so a caller-cancelled `VALIDATE CONSTRAINT` is no longer reported as a third party's cancel. `BudgetError` keeps the server's own error reachable through `Unwrap`. - `Tracker.BuildPID()` is removed. `Tracker.CancelBuild(ctx)` issues `pg_cancel_backend` over the tracker's reserved session, under the tracker's lock, only while the build is active (`ErrNoActiveBuild` after `StopConcurrentBuild`/`Finish`/`Start`/`StartStep`). The read of the backend's state and the signal are one `pg_catalog`-qualified statement (function and operators included), run detached from the caller's context on a bounded timeout so a caller deadline cannot tear down the session the build's verdict needs. A backend the server positively reports idle is `ErrBuildNotRunning`; a state the server does not expose (tracking off, hidden from the role) is `ErrBuildUnobservable`, and no signal is sent blind. A nil return means the signal was sent to a backend the same statement had just read as active — not that the build has stopped. The reserved session's role must be able to signal the backend (same role or `pg_signal_backend`); otherwise `pg_cancel_backend` raises, and the wrapped error is a permanent condition of the role, not a retryable one. - `StopConcurrentBuild` is also deferred, so the PID is retired on every exit including a panic. It is the fence that keeps `CancelBuild`'s target the build's own: the executor calls it before the build's session can return to the pool. `Start`/`StartStep`/`Finish` clear the build fields as resets under the state lock alone, so the executor's own updates never wait behind an observation (pinned by `TestStateMutatorsDoNotWaitForInFlightObservation`). Both the success and failure verdicts run under their own bounded detached context, so a build cancelled at the finish line does not report as unproven. - `Codes()` completeness is pinned by an AST test over the package's `Code` constants. - Integration tests on a real server: `CallerOwnedCallerCancel`, `CallerOwnedOperatorCancelViaTracker`, `CancelBuildResistsCatalogShadowing` (a `pg_cancel_backend` impostor ahead of `pg_catalog`), `CancelBuildRefusesAnIdleBackend`; unit tests pin the budget-over-caller precedence cell, the detached signal context, and the idle/unobservable partition. - `docs/execution-model.md` documents the precedence and the three-way partition; `docs/progress-report.md` documents the stop path and what a nil return means; `docs/capabilities.md`, `docs/tcb-model.md`, `docs/invariants.md` (LK-2), `AGENTS.md`, and the review checks stop calling a caller-owned build "bounded" and name its stop path instead: the client call is bounded, the server statement runs with `statement_timeout` off and stops only on a cancel request. ## Before / after ``` Before (#76 as merged) caller-owned build, caller's ctx ends server 57014 arrives first ──▶ ErrCancelledExternally (wrong: looks like an operator) pgx ctx error arrives first ──▶ context error (untyped) tracker.BuildPID() ──▶ pid ─ ─ ─ (build returns, pool reuses backend) ─ ─ ▶ pg_cancel_backend(pid) (hits a stranger) After any concurrent build 57014 at or past the overall budget ──▶ *BudgetError (even if caller's ctx also ended) caller's ctx ended, below the budget ──▶ ErrCancelledByCaller cancelled-by-caller 57014, caller's ctx live, below the budget──▶ ErrCancelledExternally cancelled-externally tracker.CancelBuild(ctx) (one pg_catalog-qualified statement, detached bounded ctx) build active, backend 'active' ──▶ pg_cancel_backend under the tracker's lock; nil = signal sent build active, backend idle ──▶ ErrBuildNotRunning build active, state not exposed ──▶ ErrBuildUnobservable (no blind signal) build not active ──▶ ErrNoActiveBuild (PID never leaves the tracker) ```
BuildIndexConcurrentlygains a caller-owned cancellation mode, the progress tracker becomes the operator's stop path for a running build, and progress snapshots report the lockers a concurrent build is waiting on.Why
A concurrent index build on a large table can run for hours. Today the only bound the executor accepts is a fixed server-side
statement_timeout(ConcurrentBudget.Overall), which fits a synchronous attempt but not an orchestrator that keeps a build alive for as long as it can renew a lease. That orchestrator's decision tree has three branches — my lease lapsed (retry later), an operator stopped it (do not retry), the server's budget killed it (retry with a different bound) — so the executor must report three distinguishable outcomes. Its operators also need to stop a running build without being handed a backend PID they could misuse after the build returns, and to see why a build in the "waiting for old snapshots" phase is not moving, which requires the lockers columns ofpg_stat_progress_create_index.What
ConcurrentBudget.CallerOwned: the session runs withstatement_timeout = 0and the caller's cancellable context is the statement's only bound.Overallmust be zero (ErrCallerOwnedOverallBudget), and a context that cannot be cancelled is refused withErrCallerOwnedNeedsCancellableContextbefore any session is acquired, so the statement remains bounded by construction (LK-2). The bounded mode and every existing caller are unchanged.ErrCancelledByCaller(cancelled-by-caller) whichever of the server's 57014 or the client's context error arrives first; a 57014 under a live context isErrCancelledExternally(cancelled-externally); a 57014 at the bounded mode's deadline is*BudgetError.progress.Tracker.CancelBuild(ctx)signals the active build's backend over the tracker's reserved session, under the same lock that guards the build's lifecycle and only while the build is active (ErrNoActiveBuildotherwise;ErrBuildNotRunningwhen the backend had no statement to cancel). The tracker never exposes the PID itself.dbconn.ConcurrentIndexProgressreadslockers_total,lockers_done,current_locker_pid; the snapshot carries them aswork.lockers_total/work.lockers_doneanddetail.current_locker_pid.format_versionbumps from 2 to 3 anddocs/progress-report.mddocuments the new fields and the stop path.docs/invariants.mdrecords the caller-owned exception; capability, execution-model and TCB docs describe the caller-owned context as a bound different in kind, not an absence of one.tracker.CancelBuildreturnsErrCancelledExternallywith its invalid leftover reported and the tracker then refuses a second cancel; a build whose caller cancels returnsErrCancelledByCaller, neverErrCancelledExternallyor a*BudgetError; a blocked build publisheslockers_total ≥ 1and the blocker's PID ascurrent_locker_pid.Before / after